Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add RTL support for Bootstrap CSS #16200

Merged
merged 1 commit into from
Oct 13, 2017
Merged

Add RTL support for Bootstrap CSS #16200

merged 1 commit into from
Oct 13, 2017
+163 −52

Conversation

andy-armstrong
Copy link
Contributor

@andy-armstrong andy-armstrong commented Oct 11, 2017 •
edited by marcotuts
Loading

LEARNER-1705

Description

This change implements RTL support for Bootstrap. It utilizes the library rtlcss which will convert left-to-right CSS rules into right-to-left versions.

image

Acceptance Criteria

  • Add rtlcss to the generation of Bootstrap CSS files
  • Verify that the course skeletons for LMS and Studio work with an RTL language

Sandbox

Testing

  • Unit, integration, acceptance tests as appropriate

Front-end changes:

  • i18n
  • RTL

Assign Github reviewers (as needed)

  • engineering
  • product

Post-review

  • Rebase and squash commits

@andy-armstrong andy-armstrong force-pushed the andya/bootstrap-rtl-support branch 8 times, most recently from be9aaab to 71b10db Compare October 12, 2017 19:36
@andy-armstrong andy-armstrong force-pushed the andya/bootstrap-rtl-support branch from 71b10db to 926a049 Compare October 12, 2017 20:30
marcotuts
marcotuts suggested changes Oct 12, 2017
View reviewed changes
Copy link
Contributor

@marcotuts marcotuts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue with the hero - I've enabled it on the test course after adding the verified mode and a few more course dates I stumbled across it.

image

@andy-armstrong
Copy link
Contributor Author

@marcotuts Good catch! Interestingly most of the problems are in the LTR version too. I'll fix these up tomorrow.

@marcotuts
Copy link
Contributor

Oh that makes sense. The rule breaks I saw weren't directionally specific. Cool. thumbs up technically for RTL

@andy-armstrong andy-armstrong force-pushed the andya/bootstrap-rtl-support branch from 7937e03 to fc3af68 Compare October 13, 2017 04:15
@andy-armstrong
Copy link
Contributor Author

@OmarIthawi FYI, this story implements RTL support for Bootstrap 4. I used rtlcss as discussed on the OEP (openedx/open-edx-proposals#46), and it appears to work flawlessly. One interesting thing I learned is that Bootstrap uses flexbox which automatically supports RTL in all browsers. So the layout generally just switches without any changes. Running rtlcss just ensures that other rules such as margins and padding are appropriately switched. I'd love your feedback if you see any issues with what we've built here.

OmarIthawi
OmarIthawi approved these changes Oct 13, 2017
View reviewed changes
Copy link
Member

@OmarIthawi OmarIthawi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy-armstrong I'm really excited about this! An honorary thumbs up!

I'm really impressed that this is happening. It looked like an impossible quest at the beginning. Kudos!

@OmarIthawi
Copy link
Member

BTW there should be a lot of RTL hickups here and there. This is not a show stopper at all, since it usually can be patched individually.

@andy-armstrong
Copy link
Contributor Author

Thanks @OmarIthawi. I agree that there will be patches needed for individual issues. A nice feature of rtlcss is that you can use comments to control how it does the translation:

http://rtlcss.com/learn/usage-guide/control-directives/

HarryRein
HarryRein approved these changes Oct 13, 2017
View reviewed changes
Copy link
Contributor

@HarryRein HarryRein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@andy-armstrong andy-armstrong merged commit 87b43ba into master Oct 13, 2017
@andy-armstrong andy-armstrong deleted the andya/bootstrap-rtl-support branch October 13, 2017 15:11
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, October 16, 2017.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmarIthawi OmarIthawi OmarIthawi approved these changes

@marcotuts marcotuts marcotuts approved these changes

@HarryRein HarryRein HarryRein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andy-armstrong @marcotuts @OmarIthawi @edx-pipeline-bot @HarryRein